-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
5059 bug unable to see intervention in output chart for calibrate #5110
base: main
Are you sure you want to change the base?
5059 bug unable to see intervention in output chart for calibrate #5110
Conversation
…t-chart-for-calibrate
...lient/hmi-client/src/components/workflow/ops/calibrate-ciemss/tera-calibrate-node-ciemss.vue
Outdated
Show resolved
Hide resolved
item[1].forEach((intervention) => { | ||
charts[variable].layer.push(...createInterventionChartMarkers([intervention], false, -180)); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we are applying all the interventions to the chart. We are going to end up plotting plotting interventions for variables like I, S, and R all on the same chart. It will create a issue such that chart for variable I
also have intervention for other variables, for example S
or R
along with all interventions for parameters as well. I'm not sure if that's what we want to do. If we want still draw all interventions for parameters to the chart, we should at least filter out the intervention for non target state variables (e.g. S, R etc) I guess and only plot the interventions for the parameters (e.g. beta, alpha etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is something @pascaleproulx needs to figure out with the design team. So until they do this, that PR can be a step forward, and we should merge it as a first version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I think we should include the interventions for the the same variable if it's for state variable, and all for parameters and filter out the intervention for other state variables. For example, chart for I
should include the interventions for I
only and all interventions for the parameters (e.g beta
) on top of it. @mloppie you can probably use modelPartTypesMap
in line 590 to check the type to see if a variable is state variable or a parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I still think that without proper design how we want to show parameter intervention on the state chart, just plotting all parameter interventions markers as vertical lines on the state chart won't be that useful. Parameter intervention were intentionally left out I believe. And this is going to be reworked depending on how we want to do it properly. I feel it's wise to pause until we get better idea how we should draw them on the chart from Pascale.
…t-chart-for-calibrate
…t-chart-for-calibrate
…t-chart-for-calibrate
…t-chart-for-calibrate
Fixes missing intervention marker in the calibrate node and calibrate drilldown
Resolves #5059